split: introduce configurable identity strategies - #10029
Conversation
|
Please move the "Closes" line into the relevant commit, per https://docs.jj-vcs.dev/latest/contributing/#commit-guidelines. Looks like that's the third commit, and the second commit should also link the issue. |
|
Also looks like this is your first time contributing to the repo, so you need to add your GitHub username to |
cfc6baf to
16ba66b
Compare
|
FYI @glehmann (also #10028 and discord message). |
|
Just to note it down for the future (probably out of scope for this PR): |
Agreed. I've pushed my prototype of this to split-prompt (diff). There are many subtle issues to consider, so I felt it was best to leave it for a separate PR. |
This commit refactors split.rs to prepare for identity strategies. For the follow-description strategy, we will need access to both resolved descriptions before deciding which commit inherits the original commit's change ID and bookmarks. The command previously resolved descriptions, ran the text editor, and added trailers inline while constructing each commit in sequence. The first commit was built and written before the second commit's description was resolved.
a8b99a1 to
0d39622
Compare
This introduces a new list configuration `split.identity-strategy` (defaulting to `["remaining"]`) and deprecates `split.legacy-bookmark-behavior`. - (default) `split.identity-strategy = ["remaining"]` replaces `split.legacy-bookmark-behavior = true` - `split.identity-strategy = ["selected"]` replaces `split.legacy-bookmark-behavior = false` The default behavior of `jj split` changes: * Previously: the change ID went to the commit with the selected changes and the bookmarks went to the commit with the remaining changes. * Now: both the change ID and the bookmarks go to the commit with the remaining changes. jj-vcs#10028
- If the original commit has no description, the strategy yields. - If only the selected commit matches the original description, it inherits the original commit's change ID and bookmarks. - If only the remaining commit matches the original description, it inherits the original commit's change ID and bookmarks. - If neither or both match, the strategy yields to subsequent strategies in the configured chain (falling back to `remaining` if no other strategy is configured). In non-interactive splits (for example, `jj split file` where `--editor` and `-m` are omitted), the selected commit receives the original description by default while the remaining commit receives an empty description, so `follow-description` will always assign the original change ID and bookmarks to the selected commit. Closes jj-vcs#10028
There was a problem hiding this comment.
So here's the comment I promised.
I really don't like the patchset at the moment, since we first should land #6466 and see how people respond. As I personally don't agree with the decision the maintainers made there, since they optimized it for the Git users and its backend instead of thinking about a jj-native future (OTOH, its also finally a decision after to many months but that's another issue). And since that is a clear decision on the default behavior we should just remove split.legacy-bookmark-behavior instead of treating it as something we rolled out, since it never "officially" reached users.
I personally have never been a fan of using bookmarks for Git interop (see #3402) since we've now worked at least one year in carving out their semantics in jj as its own abstraction, like the deletion of them when abandoning revisions. This to me is still doubling down on that decision which isn't great since that doesn't treat the existing workflows as something equivalent and prefers one of them over the other.
And adding additional config flags which change the default behavior for commands is something we as a project shouldn't do (see #1509) and treat treat as special exceptions (which currently only exists for next/prev --edit).
Additionally I also agree with the stuff Yuya already flagged in the issue.
There was a problem hiding this comment.
@PhilipMetzger, can you clarify exactly what about this change you don't like?
Is it the fact that strategies can be set in config flags? Would you find it acceptable to do it via CLI flags?
Is it the addition of the follow-description heuristic? Is it the enablement of the heuristic via config? Would you find it acceptable to enable the heuristic via CLI flag?
6466 makes things better for users who want branches/PRs with multiple commits, but it adds significant friction for those of us who primarily use stacked diff workflows (by taking away the config option).
Additionally I also agree with the stuff Yuya already flagged in the issue.
Yuya said:
AFAIK, we added -A/-B/-d to specify whether the new ancestor revision inherits the source change ID. 6458
Can you share how to do this? Neither I nor Antigravity could figure out how to use A/-B/-d to support a stacked diff split. Specifically I want to go from this:
@ D (descendants)
│
K (target to split, bookmarks)
│
P (parent)
to this:
@ D (descendants)
│
K" (remaining)
│
K' (selected, bookmarks, original change ID)
│
P (parent)
and I want to select the changes that to go into the parent, not the changes that go into the child.
There was a problem hiding this comment.
@PhilipMetzger, can you clarify exactly what about this change you don't like?
I mean the patch is to early not after the project has received the feedback it deserves from angry users.
Is it the fact that strategies can be set in config flags?
As Scott also alluded to in the issue, it currently is to early for them.
Would you find it acceptable to do it via CLI flags?
Ditto, this should be fine.
Is it the addition of the follow-description heuristic? Is it the enablement of the heuristic via config? Would you find it acceptable to enable the heuristic via CLI flag?
I actually didn't really at that patch yet. I personally would be careful of just copying Fig/Google Mercurial on this and think there's a better jj-native way to do it iff we're going to do it at all.
6466 makes things better for users who want branches/PRs with multiple commits, but it adds significant friction for those of us who primarily use stacked diff workflows (by taking away the config option).
Yes and that was finally a maintainer decision even though a bunch of people involved know of the pain point. And directly removing the compatibility flag incurs the wrath of the actual users (aka negative feedback) which used this and to deal with that the project will be forced to do it anyway.
IMO, the core problem here is the double duty of bookmarks as both stable symbol for CLs and as the branch pointer for Git and to solve that we correctly we should just introduce #3402.
Additionally I also agree with the stuff Yuya already flagged in the issue.
Yuya said:
AFAIK, we added -A/-B/-d to specify whether the new ancestor revision inherits the source change ID. 6458
Can you share how to do this? Neither I nor Antigravity could figure out how to use A/-B/-d to support a stacked diff split. Specifically I want to go from this:
Its not possible because the destination flags don't change anything about the bookmark behavior.
4 participants
This introduces configurable strategies for
jj splitto decide which commit gets the original change ID and bookmarks.There are three commits: the first has some refactoring, the second introduces strategies to replace
split.legacy-bookmark-behavior, and the third introducesfollow-description.Closes #10028
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.